fix(inkless): repair missing control-plane rows on CreatePartitions retry [KC-420] - #752
fix(inkless): repair missing control-plane rows on CreatePartitions retry [KC-420]#752viktorsomogyi wants to merge 1 commit into
Conversation
370f426 to
7e6c736
Compare
There was a problem hiding this comment.
Pull request overview
This PR hardens Inkless diskless-topic partition creation and consolidation behavior by making CreatePartitions retries repair missing control-plane rows and by preventing placeholder control-plane LATEST offsets from incorrectly truncating classic prefixes below the committed KRaft seal.
Changes:
- Update controller CreatePartitions handling to generate control-plane insert requests for “born-diskless” partitions on retries, including non-contiguous ranges and skipping sealed/switching partitions when the image indicates them.
- Clamp diskless LATEST offsets (and epoch end offsets derived from LATEST) to at least the committed seal to avoid truncation from placeholder rows.
- Expand unit test coverage for both behaviors across controller and diskless leader endpoint paths.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| storage/inkless/src/main/java/io/aiven/inkless/control_plane/CreateTopicAndPartitionsRequest.java | Updates API documentation to reflect range-based partition insertion semantics for retries/holes. |
| core/src/main/scala/kafka/server/ControllerApis.scala | Implements retry-aware control-plane row insertion, including partition classification and contiguous range batching. |
| core/src/test/scala/unit/kafka/server/ControllerApisTest.scala | Adds tests covering CreatePartitions retry scenarios, sealed/switching skips, non-contiguous insertion, and decrease rejection. |
| core/src/main/scala/io/aiven/inkless/consolidation/DisklessLeaderEndPoint.scala | Ensures LATEST-derived offsets are never below the committed seal and keeps leader epoch resolution consistent with the adjusted offset. |
| core/src/test/scala/io/aiven/inkless/consolidation/DisklessLeaderEndPointTest.scala | Adds tests validating seal clamping behavior for LATEST and for epoch end offsets. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…etry A failed control-plane write after KRaft already applied the increase left partitions advertised but unusable; retry returned INVALID_PARTITIONS and skipped the insert. Also treat a diskless LATEST below the committed seal as the seal, so a placeholder row cannot truncate the classic prefix. Co-authored-by: Cursor <cursoragent@cursor.com>
7e6c736 to
9f28516
Compare
jeqo
left a comment
There was a problem hiding this comment.
Looking good. Left a few comments before approving.
| verify(controlPlane).createTopicAndPartitions(ArgumentMatchers.eq(singleton( | ||
| new CreateTopicAndPartitionsRequest(topicId, topicName, 3, 4)))) | ||
| } | ||
|
|
There was a problem hiding this comment.
for completeness, we can add the scenario that should still fail and do not insert rows:
| @Test | |
| def testCreateDisklessPartitionsDecreaseRetryDoesNotInsert(): Unit = { | |
| // topic image already has 4 partitions; a decrease to 2 is rejected by KRaft with | |
| // INVALID_PARTITIONS and must NOT write control-plane rows. | |
| val topic = new CreatePartitionsTopic().setName(topicName).setAssignments(null).setCount(2) | |
| val result = new CreatePartitionsTopicResult().setName(topicName).setErrorCode(INVALID_PARTITIONS.code()) | |
| // ... arrange metadataCache image with 4 partitions for topicId (as the other retry tests do) ... | |
| // invoke createDisklessPartitions(...) | |
| verify(controlPlane, never()).createTopicAndPartitions(any()) | |
| } | |
| val retry = errorCode != Errors.NONE.code() | ||
| if (retry && count < imagePartitionCount(topicId)) { | ||
| Seq.empty | ||
| } else { | ||
| val firstPartition = | ||
| if (retry) 0 | ||
| else priorTopicStates.get(topicName) match { | ||
| case Some(state) if state.topicId == topicId => math.min(state.numPartitions, count) | ||
| case _ => 0 | ||
| } | ||
| val partitions = bornDisklessPartitions(topicId, count).filter(_ >= firstPartition) |
There was a problem hiding this comment.
imagePartitionCount and bornDisklessPartitions both access metadataCache.currentImage() snapshot at different points, potentially getting different images; maybe:
| val retry = errorCode != Errors.NONE.code() | |
| if (retry && count < imagePartitionCount(topicId)) { | |
| Seq.empty | |
| } else { | |
| val firstPartition = | |
| if (retry) 0 | |
| else priorTopicStates.get(topicName) match { | |
| case Some(state) if state.topicId == topicId => math.min(state.numPartitions, count) | |
| case _ => 0 | |
| } | |
| val partitions = bornDisklessPartitions(topicId, count).filter(_ >= firstPartition) | |
| val retry = errorCode != Errors.NONE.code() | |
| val imagePartitions = Option(metadataCache.currentImage().topics().getTopic(topicId)) | |
| .map(_.partitions()) | |
| .getOrElse(util.Collections.emptyMap()) | |
| if (retry && count < imagePartitions.size()) { | |
| Seq.empty | |
| } else { | |
| val firstPartition = | |
| if (retry) 0 | |
| else priorTopicStates.get(topicName) match { | |
| case Some(state) if state.topicId == topicId => math.min(state.numPartitions, count) | |
| case _ => 0 | |
| } | |
| val partitions = bornDisklessPartitions(imagePartitions, count).... |
| val endPoint = listOffsetEndPointWithPlaceholderEpoch(offset = 10L, seal = 150000L, disklessLeaderEpoch = 5) | ||
| assertEquals(new OffsetAndEpoch(150000L, 5), endPoint.fetchLatestOffset(topicPartition, 3)) | ||
| } | ||
|
|
There was a problem hiding this comment.
for completeness, adding the negative
| @Test | |
| def testFetchLatestOffsetSwitchPendingSealIsUnchanged(): Unit = { | |
| val endPoint = listOffsetEndPointWithPlaceholderEpoch( | |
| offset = 0L, | |
| seal = PartitionRegistration.CLASSIC_TO_DISKLESS_SWITCH_PENDING, | |
| disklessLeaderEpoch = PartitionRegistration.NO_DISKLESS_LEADER_EPOCH) | |
| assertEquals(new OffsetAndEpoch(0L, 0), endPoint.fetchLatestOffset(topicPartition, 3)) | |
| } | |
| @Test | |
| def testFetchLatestOffsetBornDisklessSealIsUnchanged(): Unit = { | |
| val endPoint = listOffsetEndPointWithPlaceholderEpoch( | |
| offset = 0L, | |
| seal = PartitionRegistration.NO_CLASSIC_TO_DISKLESS_START_OFFSET, | |
| disklessLeaderEpoch = PartitionRegistration.NO_DISKLESS_LEADER_EPOCH) | |
| assertEquals(new OffsetAndEpoch(0L, 0), endPoint.fetchLatestOffset(topicPartition, 3)) | |
| } | |
| @Test | |
| def testFetchLatestOffsetExactlyAtSealIsUnchanged(): Unit = { | |
| val endPoint = listOffsetEndPointWithPlaceholderEpoch(offset = 150000L, seal = 150000L, disklessLeaderEpoch = 5) | |
| assertEquals(new OffsetAndEpoch(150000L, 5), endPoint.fetchLatestOffset(topicPartition, 3)) | |
| } | |
| .filter { case (_, res) => | ||
| val code = res.errorCode() | ||
| code == Errors.NONE.code() || | ||
| code == Errors.TOPIC_ALREADY_EXISTS.code() || |
There was a problem hiding this comment.
does this error actually need to be covered? I think it comes from topic creation which is not reachable when creating partitions
A failed control-plane write after KRaft already applied the increase left partitions advertised but unusable; retry returned INVALID_PARTITIONS and skipped the insert. Also treat a diskless LATEST below the committed seal as the seal, so a placeholder row cannot truncate the classic prefix.